I admit I'm not super excited about exposing the failure code as-is for two reasons - (a) we have very different constraints on the code - we may not always want to give the sender all the details about a failure (eg if the sender used a private channel but we didn't mean to expose it we might pretend the channel doesnt exist) vs what we want to tell the user about a failure (basically everything), (b) its not particularly easy to parse - users have to go read the BOLT then map that to what is actually happening in LDK code, if we had a big enum here we'd be able to provide good docs linking to config knobs and reasons why failures happened.

Makes sense, will take a look at adding a more detailed enum or adding to HTLCDestination 👍

we may not always want to give the sender all the details about a failure (eg if the sender used a private channel but we didn't mean to expose it we might pretend the channel doesnt exist)

Maybe I don't understand this fully, but why would you want to hide information from a local api user?

The private channel example is a case where the BOLT 04 code hides information from the sender of the payment, but we do want to surface that information on the API (comment is from a previous approach that just surfaced the BOLT 04 failure code, so wouldn't provide this additional information).

why would you want to hide information from a local api user?

I do think there are some cases where the end user probably doesn't care about very detailed errors. For example, InvalidOnionPayload - do we really care whether the version is unknown or the onion key is bad when there's nothing that we can do about it?

I see, outdated.

About your second remark - the end user is a user of the library, I assume. Maybe hard to know what they care about. But still it's probably more efficient to wait for someone asking for it than it is to add it potentially unnecessarily. In that context interested to know whether the failure code itself is useful? (posted #3541 (comment))

About your second remark - the end user is a user of the library, I assume.

Yeah, whoever is consuming LDK's events 👍

But still it's probably more efficient to wait for someone asking for it than it is to add it potentially unnecessarily.

Complaint driven development FTW 😅

In that context interested to know whether the failure code itself is useful?

As discussed on call, this does fall into the nice to have category. I do also think it's also a nice readability improvement to get rid of the failure codes scattered through the codebase, reduces the chances of using the wrong values IMO.

Its worth noting that HTLCDestination has some "reason"-like attributes (I meant to mention this in the issue, but apparently forgot), they're just incomplete and too broad. Up to you if you want to try to add more details there vs adding a new enum.

Hmmm, rather than having a variant that just holds the error code (and exposing it), can we just convert every use of error codes to the enum version and then only write it out as a u16 when we go to serialize the failure? This would also have the nice side-effect of ensuring that we have all the error codes we generate written in one place instead of strewn all over the codebase.

Maybe I'm not quite sure I understand why you needed to do this?

an we just convert every use of error codes to the enum version and then only write it out as a u16 when we go to serialize the failure

Yeah can def do that. I thought that some of the error codes were overly-specific (so not something that we want to expose to the end user), but if that's okay then it's a much simpler approach.

by the local node

Could this potentially be reused later on when exposing the failure reason on the origin side? If so, should the naming be more generic?

Outbound payments failure reasons are already reasonably covered byPaymentFailureReason IMO so going to leave as-is for now, especially because the original sender would only get the (less interesting) subset of variants that map directly to bolt 04 codes.

I'm wondering if it makes sense to create a specific error for this tuple. I'm not familiar enough with this part of the code base to say that we can add another kind of ChannelError, but something on these lines?

It looks like we'll return this in can_accept_incoming_htlc, and the docs on that method state that the HTLC has already been irrevocably committed to when the method is called, which seems to contradict this a bit.

I'm also wondering if this variant can be named ChannelClosed instead? It seems like the channel closing is the real reason that DroppedPending HTLCs failed. The current ChannelClosed variant seems more like OnchainTimeout, but I'm still looking through the code on this one to confirm.

nit: s/Liquidity/HTLC on these might be clearer?

Originally decided agianst this because I think HTLC(min/max) sounds like we've hit channel policy limits on min/max, when get_available_balances actually incorporate all sorts of things (dust, policy, liquidity etc).

Going with the rename + adding more info to the docs instead 👍

Nice to get rid of these unreachable code paths!

I think this is off because we need to write a u16 here? Unfortunately our ser macros currently don't enforce that the type being written matches the legacy type :(

I think for safety we should return the unknown failure reason here even though (as you note above) the failure_code should always be set in this situation

Changed this to return InvalidValue if reason is None and we don't have a _failure_code, similar to the way it's been done for attribution_data above.

Open to other approaches here too! Still getting comfortable w/ these macros.

Makes sense, thanks! Definitely a bit of a learning curve to these macros at times

Looks like this is an existing bug in the codebase: https://github.com/lightning/blips/blob/master/blip-0003.md and we should be erroring with PERM|15.

Pulled out the fix for this in 11a4f3e then updated in the refactor 👍

nit: we're trying to adopt rustfmt formatting where possible ahead of actually using it on this file, which would format as such:

fn construct_pending_htlc_fail_msg<'a>(
    &self, msg: &msgs::UpdateAddHTLC, counterparty_node_id: &PublicKey,
    shared_secret: [u8; 32], inbound_err: InboundHTLCErr,
) -> HTLCFailureMsg {
    ...
}

nit: separate this new call into an intermediate var?

Looks like another benefit of this refactor is that we can more easily remove the PendingHTLCStatus struct once its legacy usage in channel.rs is removed

Even though this commit is small, still separating the refactor from the bug fix is beneficial I think.

I wonder if this should return a (code, data) tuple, to prevent bugs where the data is used with a diff code.

Returning the code isn't helpful in the commit that follow this because we start to introduce variants like InvalidKeysendPreimage so this function can't always return IncorrectPaymentDetails.

Could isolate the rename. This commit already needs a lot of attention to scan for accidental replacement errors.

I had to think of a map here for readability, but no idea of the implications of that and whether it is worth it.

I tried something like this here. The downside of a map is that you can have a runtime failure to find the code you're looking for if you forget to add something to the map; I felt that the compiler check that match has all the variants was more compelling than the readability improvement.

Explicit discriminants were also suggested, but they require a unique value per variant.

Could also have this be something like this, which has the benefit of only defining the values once:

if self == LocalHTLCFailureReason::TemporaryNodeFailure.failure_code() {
			LocalHTLCFailureReason::TemporaryNodeFailure
		}

Looks a little ungodly to me, but happy to consider something different here (I don't love it either).

Could be:

macro_rules! impl_into_u16_for_local_failure {
    ($($variant:path),+ $(,)?) => {
        impl Into<LocalHTLCFailureReason> for u16 {
            fn into(self) -> LocalHTLCFailureReason {
                $(
                    if self == $variant.failure_code() {
                        return $variant;
                    }
                )+
                LocalHTLCFailureReason::UnknownFailureCode { code: self }
            }
        }
    };
}

impl_into_u16_for_local_failure!(
    LocalHTLCFailureReason::TemporaryNodeFailure,
    LocalHTLCFailureReason::PermanentNodeFailure,
   ... all bolt 04 variants
)

Not necessarily advocating this, but you could use the macro to define the enum as well. This would ensure each one is used, whereas otherwise impl_into_u16_for_local_failure needs to be updated whenever a new variant is added.

For some creative uses of macros see:

• https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-types/src/features.rs
• https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-custom-message/src/lib.rs

• rust-lightning/lightning/src/util/ser_macros.rs

  Line 1050 in 83e9e80

  macro_rules! tlv_stream {

For instance, you could use a pared down version of composite_custom_message_handler's rule to specify each variant as something like:

macro_rules! failure_reason {
	($($variant:ident($code:expr)),* $(,)*) => {
		// ...
	}
}

failure_reason!(
	TemporaryNodeFailure(UPDATE | 7),
	// ...
)

Adding docs may not be as nice. We do something for features like this:

Ok, if you all start threatening with macros, I'll back off 😅

another rename to isolate potentially

This commit is just so intense. Getting nervous of reviewing so many trivial changes. I wonder if it is worth to:

• Do the renames from err_code to reason separately in a non-review commit
• Introduce the LocalHTLCFailureReason enum, still unused
• Have a commit that contains just mechanical search/replaces that needs no review. Possibly list all the replaces that were applied.
• Then finish with a commit with the semi-manual leftovers.

Maybe it's too much of an ask though.

I'll take a shot at splitting this up 👍

Perhaps some explanation in the commit message or code why we don't just keep writing a u16 code?

This seems to be a fundamental design choice. To allow multiple reasons to map to the same bolt code. Somehow it suggests a layering violation, but perhaps this is just the practical way to do it.

It does make this commit more than the basic 'replace u16 with enum'.

This seems to be a fundamental design choice. To allow multiple reasons to map to the same bolt code.

Indeed, this is one variant because it allows us to surface this enum directly on the API. Without that, we'd need some sort of wrapper enum which allows either a direct BOLT 04 mapping or an "enriched" failure that then maps to the BOLT04 mapping.

enum Failure {
    Bolt04(Bolt04Failure)
   Enriched(LocalFailure)
}

Since we want to surface additional information about the errors, we have to accept that this many to one mapping has to happen somewhere - it either happens in one enum, or across two.

The above Failure structure also doesn't really make sense to surface to the end user, so we'd then need to map this further to a flat enum, which adds a lot of boiler plate.

Somehow it suggests a layering violation, but perhaps this is just the practical way to do it.

While it does feel a bit like a layering violation because these values end up on the API, it is also just a more accurate way of representing internal error states. The BOLT04 error codes erase a lot of information that's internally available, so perhaps the actual layering violation is to use wire-level error codes internally? For example, this change also gives us more precise unit testing because we can now assert that the TemporaryChannelFailure we're receiving is actually the one we're expecting.

The BOLT04 error codes erase a lot of information that's internally available, so perhaps the actual layering violation is to use wire-level error codes internally?

I love this inversion. Sold.

Again a bit of that layering friction, at least that is how I perceive it.

Prefer From over Into as per the Into docs: https://doc.rust-lang.org/std/convert/trait.Into.html